-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(ec2): dual-stack vpc without private subnets creates EgressOnlyInternetGateway (under feature flag) #34437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ec2): dual-stack vpc without private subnets creates EgressOnlyInternetGateway (under feature flag) #34437
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
82ac2e3
to
a0d833f
Compare
7332c64
to
427b4a2
Compare
For the PR title please change it to describe the bug and not the actual fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work with this change! Just a few comments regarding the naming of the feature flag.
@@ -137,6 +137,7 @@ export const DYNAMODB_TABLE_RETAIN_TABLE_REPLICA = '@aws-cdk/aws-dynamodb:retain | |||
export const LOG_USER_POOL_CLIENT_SECRET_VALUE = '@aws-cdk/cognito:logUserPoolClientSecretValue'; | |||
export const PIPELINE_REDUCE_CROSS_ACCOUNT_ACTION_ROLE_TRUST_SCOPE = '@aws-cdk/pipelines:reduceCrossAccountActionRoleTrustScope'; | |||
export const S3_TRUST_KEY_POLICY_FOR_SNS_SUBSCRIPTIONS = '@aws-cdk/s3-notifications:addS3TrustKeyPolicyForSnsSubscriptions'; | |||
export const ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC = '@aws-cdk/ec2:removeEgressOnlyGatewayFromPublicSubnetVPC'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know it's not consistent but the convention is to prefix the service here with aws-
. Also we should prefix the constant with the module name EC2-
and update the name of the feature flag to more accurately reflect the behaviour.
export const ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC = '@aws-cdk/ec2:removeEgressOnlyGatewayFromPublicSubnetVPC'; | |
export const EC2_REQUIRE_PRIVATE_SUBNETS_FOR_EGRESSONLYINTERNETGATEWAY = '@aws-cdk/aws-ec2:requirePrivateSubnetsForEgressOnlyInternetGateway'; |
@@ -1576,6 +1577,17 @@ export const FLAGS: Record<string, FlagInfo> = { | |||
}, | |||
|
|||
////////////////////////////////////////////////////////////////////// | |||
[ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC]: { | |||
type: FlagType.BugFix, | |||
summary: 'Remove EgressOnlyGateway resource when a a double stack vpc has only public subnets', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem entirely correct. Rather than "removing EgressOnlyGateway
when a stack only has public subnets", a more accurate summary could be "When enabled, the EgressOnlyGateway resource is only created if private subnets are defined in the VPC."
@@ -1649,7 +1649,13 @@ export class Vpc extends VpcBase { | |||
} | |||
|
|||
// Create an Egress Only Internet Gateway and attach it if necessary | |||
if (this.useIpv6 && this.privateSubnets) { | |||
|
|||
const redundantEgressOnlyGatewayRemovalFeatureFlag = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: variable name
const redundantEgressOnlyGatewayRemovalFeatureFlag = | |
const isRequirePrivateSubnetsForEgressOnlyIgw = |
@@ -2747,6 +2747,90 @@ describe('vpc', () => { | |||
}, | |||
}); | |||
}); | |||
test('(default)EgressOnlyInternetGateWay is created when no private subnet configured in dual stack', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of (default) EgressOnlyIGW....
maybe change this to say "EgressOnlyIGW is created when....in dual stack when feature flag is enabled."
// THEN | ||
Template.fromStack(stack).resourceCountIs('AWS::EC2::EgressOnlyInternetGateway', 1); | ||
}); | ||
test('(default)EgressOnlyInternetGateWay is created when private subnet configured in dual stack', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above test.
Template.fromStack(stack).resourceCountIs('AWS::EC2::EgressOnlyInternetGateway', 1); | ||
}); | ||
|
||
test('(feature flag ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC)EgressOnlyInternetGateWay is created when private subnet configured in dual stack', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
test('(feature flag ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC)EgressOnlyInternetGateWay is created when private subnet configured in dual stack', () => { | |
test('EgressOnlyInternetGateWay is created when private subnet configured in dual stack when feature flag is enabled', () => { |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Co-authored-by: paulhcsun <[email protected]>
…under feature flag) (aws#33702) V3 but I think we got there Closes aws#32811 By default when you create an s3 bucket, all public access is already blocked. However if you then use CDK to specify 1 or more access point you want to unblock, all undefined block types will be auto set to false, and when it deploys you will see everything uncheck even if you only wanted to uncheck 1 thing. So to fix this we should instead default all values to true when at least 1 option is specified, to mimic to experience when a user in the console unchecks the boxes. deprecating `BLOCK_ACLS` method of `BlockPublicAccess`. Adding `BLOCK_ACLS_ONLY`. ``` public static readonly BLOCK_ACLS_ONLY = new BlockPublicAccess({ blockPublicAcls: true, blockPublicPolicy: false, ignorePublicAcls: true, restrictPublicBuckets: false, }); ``` This is just a general revamp to match what the feature will bring, it's separate from the feature itself. The point being that for any shortcut methods like this, we should be specifying all 4 options to ensure the default true behavior remains. Created function `setBlockPublicAccessDefaults()` ``` /** * Function to set the blockPublicAccessOptions to a true default if not defined. * If no blockPublicAccessOptions are specified at all, this is already the case as an s3 default in aws * @see https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-control-block-public-access.html */ private setBlockPublicAccessDefaults(blockPublicAccessOptions: BlockPublicAccessOptions) { return { blockPublicAcls: blockPublicAccessOptions.blockPublicAcls ?? true, blockPublicPolicy: blockPublicAccessOptions.blockPublicPolicy ?? true, ignorePublicAcls: blockPublicAccessOptions.ignorePublicAcls ?? true, restrictPublicBuckets: blockPublicAccessOptions.restrictPublicBuckets ?? true, }; } ``` but this method is only called if the FF is enabled ``` let blockPublicAccess: BlockPublicAccessOptions | undefined = props.blockPublicAccess; if (props.blockPublicAccess && FeatureFlags.of(this).isEnabled(cxapi.S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE)) { blockPublicAccess = this.setBlockPublicAccessDefaults(props.blockPublicAccess); } ``` Of course the FF itself was added. Added tests that are duplicates of others, just testing for both behaviors with and without the FF. ``` describe('bucket with custom block public access setting', () => { ... test('S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE Enabled', () => { const app = new cdk.App({ context: { [cxapi.S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE]: true, }, }); const stack = new cdk.Stack(app); new s3.Bucket(stack, 'MyBucket', { blockPublicAccess: new s3.BlockPublicAccess({ restrictPublicBuckets: false }), }); Template.fromStack(stack).templateMatches({ 'Resources': { 'MyBucketF68F3FF0': { 'Type': 'AWS::S3::Bucket', 'Properties': { 'PublicAccessBlockConfiguration': { 'BlockPublicAcls': true, 'BlockPublicPolicy': true, 'IgnorePublicAcls': true, 'RestrictPublicBuckets': false, }, }, 'DeletionPolicy': 'Retain', 'UpdateReplacePolicy': 'Retain', }, }, }); }); ``` ``` describe('bucket with custom block public access setting', () => { ... test('S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE Enabled', () => { const app = new cdk.App({ context: { [cxapi.S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE]: true, }, }); const stack = new cdk.Stack(app); new s3.Bucket(stack, 'MyBucket', { blockPublicAccess: new s3.BlockPublicAccess({ restrictPublicBuckets: false }), }); Template.fromStack(stack).templateMatches({ 'Resources': { 'MyBucketF68F3FF0': { 'Type': 'AWS::S3::Bucket', 'Properties': { 'PublicAccessBlockConfiguration': { 'BlockPublicAcls': true, 'BlockPublicPolicy': true, 'IgnorePublicAcls': true, 'RestrictPublicBuckets': false, }, }, 'DeletionPolicy': 'Retain', 'UpdateReplacePolicy': 'Retain', }, }, }); }); ``` Also added an integ that just tests different combinations of the blocking. https://github.com/aws/aws-cdk/blob/51ffe2112e048f5866e5c0d811377b4deca7920d/packages/%40aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-block-access.ts#L1-L42 There was no `BlockPublicAccess` integ before so I did not add the context for the FF disabled anywhere. The tests should still be working since it's not used that often. But if the team needs me to, I can add a 2nd integ with the old behavior - [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* rebased main
resolved merge conflict
resolved merge conflict resolved merge conflicts
13e9d1b
to
e45ac63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work!
@Mergifyio update |
✅ Branch has been successfully updated |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #30981.
Reason for this change
-> EgressOnlyInternetGateway was been created even without any private subnets
Description of changes
-> Fixed the condition that determins if a EgressOnlyInternetGateway will be created
-> Added feature flag
Describe any new or updated permissions being added
N/A
Description of how you validated changes
I added two new unit tests that checks if EgressOnlyInternetGateway is created without a private subnet
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license